docs(immich): sync template standards updates#354
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds immich network policy playground fields, enables immich in the playground chart list, and updates the immich values documentation table. ChangesImmich playground and docs
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/data/playground-configs.ts (1)
236-262: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDuplicate "Network Policy" section for apache.
Two separate field groups named
Network Policy, both gated bynetworkPolicy.enabled, now exist in the same chart config array (lines 236-262 and 285-305). This renders two identically-titled collapsible sections in the playground UI instead of one, which is confusing and likely unintended — the new egress CIDR/port fields should be merged into the existing group.♻️ Proposed fix: merge into the existing Network Policy group
{ name: 'Network Policy', collapsible: true, gateField: 'networkPolicy.enabled', fields: [ { label: 'Restrict Ingress', key: 'networkPolicy.ingress.enabled', type: 'toggle', default: 'false', description: 'Restrict inbound traffic only when selectors are configured', }, { label: 'Allow DNS', key: 'networkPolicy.egress.allowDns', type: 'toggle', default: 'true', description: 'Allow DNS egress', }, { label: 'Allow Internet', key: 'networkPolicy.egress.allowInternet', type: 'toggle', default: 'true', description: 'Allow general outbound traffic', }, + { + label: 'Extra Egress CIDR', + key: 'networkPolicy.extraEgress[0].to[0].ipBlock.cidr', + type: 'text', + default: '10.0.0.0/8', + description: 'Additional destination allowed by NetworkPolicy', + }, + { + label: 'Extra Egress Port', + key: 'networkPolicy.extraEgress[0].ports[0].port', + type: 'number', + default: '443', + description: 'Additional TCP egress port', + }, ], }, ... - { - name: 'Network Policy', - collapsible: true, - gateField: 'networkPolicy.enabled', - fields: [ - { - label: 'Extra Egress CIDR', - key: 'networkPolicy.extraEgress[0].to[0].ipBlock.cidr', - type: 'text', - default: '10.0.0.0/8', - description: 'Additional destination allowed by NetworkPolicy', - }, - { - label: 'Extra Egress Port', - key: 'networkPolicy.extraEgress[0].ports[0].port', - type: 'number', - default: '443', - description: 'Additional TCP egress port', - }, - ], - },Also applies to: 285-305
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/data/playground-configs.ts` around lines 236 - 262, The playground config defines two separate collapsible groups named Network Policy in the same chart array, both using networkPolicy.enabled, which creates duplicate UI sections. Merge the new egress CIDR/port fields into the existing Network Policy group in playground-configs.ts rather than keeping a second group, and keep the unique field keys under the same fields array so the apache config shows only one section.src/pages/playground.astro (1)
23-32: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd Immich’s Network Policy controls to the curated playground config
src/pages/playground.astro:23-32
Addingimmichhere only makes the chart selectable in the playground; the curated Immich config still overrides schema-generated fields and has noNetwork Policygroup, sonetworkPolicy.extraEgresswon’t surface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/playground.astro` around lines 23 - 32, Update the curated Immich playground config so it includes the Network Policy controls instead of only adding immich to siteSyncPlaygroundConfigs. In the playground config source used by immich, preserve the schema-generated Network Policy group and ensure networkPolicy.extraEgress is exposed rather than being overridden by custom curated fields. Use the immich entry and the playground config definitions to locate the override and merge in the missing group.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/data/playground-configs.ts`:
- Around line 236-262: The playground config defines two separate collapsible
groups named Network Policy in the same chart array, both using
networkPolicy.enabled, which creates duplicate UI sections. Merge the new egress
CIDR/port fields into the existing Network Policy group in playground-configs.ts
rather than keeping a second group, and keep the unique field keys under the
same fields array so the apache config shows only one section.
In `@src/pages/playground.astro`:
- Around line 23-32: Update the curated Immich playground config so it includes
the Network Policy controls instead of only adding immich to
siteSyncPlaygroundConfigs. In the playground config source used by immich,
preserve the schema-generated Network Policy group and ensure
networkPolicy.extraEgress is exposed rather than being overridden by custom
curated fields. Use the immich entry and the playground config definitions to
locate the override and merge in the missing group.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bd0c49e2-ab2a-468e-9710-272a7f746575
📒 Files selected for processing (3)
src/data/playground-configs.tssrc/pages/docs/charts/immich.mdxsrc/pages/playground.astro
|
Verified the CodeRabbit review-body feedback against the current branch. Result:
Validation:
No code change was needed because the review-body findings are already resolved in the current PR state. |
## Summary - Keep the Immich Helm test hook under `templates/tests/` so `helm test` renders and executes it, with unittest coverage for the hook pod. - Add `networkPolicy.extraEgress` and unit coverage. - Rewrite NOTES into numbered operational sections. - Remove decorative status icons from the touched README security scan block. ## Related - Site sync: helmforgedev/site#354. - Refs #633. ## Validation - `helm template test charts/immich | rg -n "helm.sh/hook|test-connection"` (hook rendered from `templates/tests/test-connection.yaml`) - `helm unittest charts/immich` (45 tests, 8 suites) - `make template-standards-check CHART=immich` - `node scripts/charts/validate-chart.js --chart immich --no-k3d` - `make validate-chart CHART=immich TIMEOUT=900` (FULLY VALIDATED, 12 layers) - `make site-sync-check CHART=immich` - `make release-check REPO=charts` - `make attribution-check REPO=charts` ## Site Validation - `npm run lint` - `npm run format:check` - `npm run build` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for extra outbound NetworkPolicy rules via `networkPolicy.extraEgress`, allowing additional egress destinations and ports. * **Documentation** * Updated the chart README with a new NetworkPolicy section and refined the security scan formatting. * Reworked the chart NOTES/help output into clearer numbered sections with additional troubleshooting guidance. * **Tests** * Expanded NetworkPolicy test coverage to confirm `extraEgress` is appended after the base egress rules. * Added a Helm connection test suite to validate the rendered test pod. * **Chores** * Extended the NetworkPolicy values schema and defaults to include `enabled`, `ingress`, `egress`, and `extraEgress`. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: MergeCheck <mergecheck@example.invalid>
6281a30 to
d755523
Compare
Summary
Validation
Chart PR: helmforgedev/charts#676
Summary by CodeRabbit
networkPolicy.extraEgresssetting.